Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace 'johnson' with ExecJS #1

Merged
merged 6 commits into from
Jul 23, 2011
Merged

Replace 'johnson' with ExecJS #1

merged 6 commits into from
Jul 23, 2011

Conversation

ArthurN
Copy link
Contributor

@ArthurN ArthurN commented Jul 18, 2011

In the process of trying to integrate therubyracer to support Ruby 1.9.2 per your blog comment, I ended up discovering ExecJS, which can work with any number of JS runtimes including both johnson and therubyracer. That seemed like the better solution, so here it is. As such, it ends up being up to the user of Isotope to select the runtime to work with ExecJS.

Additionally, I fixed a small bug in the spec (timezone-vs-UTC dates) and updated the Date.parse code to the latest version from this repo. The example application is also updated. I have NOT updated the Readme, however.

I've tested this with:
1.8.7 using johnson
1.8.7 using therubyracer
1.9.2 using therubyracer

on both the spec and the example app.

Cheers!

@elado
Copy link
Owner

elado commented Jul 18, 2011

Thanks!
Didn't have the chance to look deeper into the code, but the test succeeds on 1.8.7-p334 and fails on 1.9.2. Can you check?

$ rspec test/isotope_spec.rb 
rewriting path constants for test...
/Users/Elad/Sites/temp/isotope/test/isotope_spec.rb:7: warning: already initialized constant APP_ROOT
/Users/Elad/Sites/temp/isotope/test/isotope_spec.rb:8: warning: already initialized constant DEFAULT_CONFIG_PATH
.FF

Failures:

  1) Isotope should render articles
     Failure/Error: evaluated_content = Isotope.render_partial(@template_file, :locals => { :item => @articles[0] })
     ExecJS::ProgramError:
       ReferenceError: formatDate is not defined
     # ./lib/isotope/isotope.rb:44:in `render_partial'
     # ./test/isotope_spec.rb:45:in `block (2 levels) in <top (required)>'

  2) Isotope should render an array of articles
     Failure/Error: evaluated_content = Isotope.render_partial(@template_file, :collection => @articles, :delimiter => "<hr/>")
     ExecJS::ProgramError:
       ReferenceError: formatDate is not defined
     # ./lib/isotope/isotope.rb:44:in `render_partial'
     # ./lib/isotope/isotope.rb:57:in `block in render_partial_collection'
     # ./lib/isotope/isotope.rb:52:in `each'
     # ./lib/isotope/isotope.rb:52:in `render_partial_collection'
     # ./lib/isotope/isotope.rb:35:in `render_partial'
     # ./test/isotope_spec.rb:65:in `block (2 levels) in <top (required)>'

Finished in 0.1307 seconds
3 examples, 2 failures

@ArthurN
Copy link
Contributor Author

ArthurN commented Jul 18, 2011

What runtime are you using on 1.9.2, johnson or therubyracer?

Note that I intentionally did not require any runtime gem in isotope's Gemfile. My thinking was that the application will include whatever runtime it prefers in the Gemfile in addition to isotope itself. However, for simplicity's sake, we could just include 'therubyracer' in the isotope Gemfile. I'd have to double-check, but I think that ExecJS will prefer therubyracer over johnson if both are available on the system.

@ArthurN
Copy link
Contributor Author

ArthurN commented Jul 18, 2011

The reason I ask, of course, is because it seems to be working for me:

[507][arthur.Inception: isotope]$ rvm current
ruby-1.9.2-p180
[508][arthur.Inception: isotope]$ gem list | grep johnson
[509][arthur.Inception: isotope]$ gem list | grep rubyracer
therubyracer (0.9.3beta1, 0.9.2)
[510][arthur.Inception: isotope]$ rspec test/isotope_spec.rb 
rewriting path constants for test...
/Users/arthur/Projects/isotope/test/isotope_spec.rb:7: warning: already initialized constant APP_ROOT
/Users/arthur/Projects/isotope/test/isotope_spec.rb:8: warning: already initialized constant DEFAULT_CONFIG_PATH
...

Finished in 0.06601 seconds
3 examples, 0 failures

@elado
Copy link
Owner

elado commented Jul 21, 2011

OK, the problem was that I didn't have therubyracer gem on my 1.9. Please add it to the Gemfile and iI'll merge.
Thanks!

@ArthurN
Copy link
Contributor Author

ArthurN commented Jul 23, 2011

Will take care of that, as well as a small bug I discovered, tomorrow. Thanks!

@ArthurN
Copy link
Contributor Author

ArthurN commented Jul 23, 2011

Updated

@ArthurN
Copy link
Contributor Author

ArthurN commented Jul 23, 2011

Good call. Updated.

elado added a commit that referenced this pull request Jul 23, 2011
@elado
Merge pull request #1 from ArthurN/master
4243fdd 
Replace 'johnson' with ExecJS
@elado elado merged commit 4243fdd into elado:master Jul 23, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ArthurN @elado